Skip to content

[test] add fuzz targets for sequences of tbor commands and dispatcher - #787

Open
David Zimmermann (zimmy87) wants to merge 10 commits into
mainfrom
user/v-davidz/add_tbor_app_cmd_dispatcher
Open

David Zimmermann (zimmy87) wants to merge 10 commits into
mainfrom
user/v-davidz/add_tbor_app_cmd_dispatcher

Conversation

@zimmy87

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI balanced review requested due to automatic review settings October 9, 2026 19:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new targets can silently select and repeatedly factory-reset a persistent hardware device.

4 open findings
What changed in this PR

Adds four TBOR fuzz targets covering command sequences, ECC dispatch, AES fast paths, and session operations.

Changes:

  • Adds and registers four fuzz binaries.
  • Exercises valid and malformed TBOR requests.
  • Reformats existing policy-key construction.
File Description
fuzz/​Cargo.toml Registers the new fuzz targets.
fuzz/​fuzz_targets/​common.rs Formatting-only change.
fuzz_tbor_cmd_reqs.rs Fuzzes command sequences.
fuzz_tbor_dispatcher.rs Fuzzes ECC request dispatch.
fuzz_tbor_dispatcher_aes_fp.rs Fuzzes AES fast-path inputs.
fuzz_tbor_app_ops.rs Fuzzes session operations.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_app_ops.rs
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_cmd_reqs.rs
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_dispatcher.rs
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_dispatcher_aes_fp.rs
Copilot AI balanced review requested due to automatic review settings October 9, 2026 20:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_app_ops.rs Outdated
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_cmd_reqs.rs
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_dispatcher.rs
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_app_ops.rs Fixed
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_app_ops.rs Fixed
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_app_ops.rs Fixed
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_app_ops.rs Fixed
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_dispatcher_aes_fp.rs Fixed
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 9, 2026 22:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The command-sequence target incorrectly accepts ECC and HMAC driver failures as expected invalid-session rejections.

0 open findings

3 resolved since last review

🧠 Review effort: Balanced

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 9, 2026 23:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The app-operations target cannot reach its main paths, and two command branches suppress driver failures.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Medium severity Secondary CO sessions blocked by exhausted bootstrap session slots

fuzz/​fuzz_targets/​ddi/​tbor/​fuzz_tbor_app_ops.rs:61

The bootstrap session is an active Crypto-Officer session, and CO handshakes have only slot 0 available. Consequently every secondary CO SessionOpenInit reaches VaultSessionLimitReached here and immediately continues, so no finish-MAC or close operation below is ever exercised. Use the rotated CU fixture/credentials for the primary and secondary handles (CU has slots 1–7), or close the bootstrap CO and restructure the per-handle-limit check before opening secondaries.

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 11, 2026 00:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

ECC and HMAC branches can incorrectly suppress driver failures as expected request rejections.

0 open findings

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 11, 2026 04:08
@zimmy87
David Zimmermann (zimmy87) marked this pull request as ready for review October 11, 2026 04:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The app-operations target cannot reach its core session logic, and two command branches suppress driver failures.

0 open findings

Previously missed (2)

In code that hasn't changed since last review

Medium severity Secondary CO session always exceeds the firmware session limit

fuzz/​fuzz_targets/​ddi/​tbor/​fuzz_tbor_app_ops.rs:61

The active bootstrap_rotated_co session already occupies the firmware's single CO slot, so this secondary CO SessionOpenInit always returns VaultSessionLimitReached (the same invariant is exercised in ddi/tbor/types/tests/commands/open_session.rs:352-372). Consequently every OpenSession operation continues here, file_handles remains empty, and none of the Phase 2 or close-session sequence logic is fuzzed. Use a session type/identity with spare capacity (for example CU), or restructure the bootstrap so the CO slot is free before opening the secondary session.

Medium severity ECC DriverError is silently swallowed

fuzz/​fuzz_targets/​ddi/​tbor/​fuzz_tbor_cmd_reqs.rs:132

This wildcard converts ECC DriverError results into an ordinary request failure. For invalid session IDs or unsupported scopes, the later assertion expects false, so an I/O abort is silently accepted instead of being reported as the crash this fuzz target is meant to detect; the AES branch above and the other TBOR fuzz targets explicitly panic on DriverError. Preserve that arm before the wildcard.

This issue also appears on line 148 of the same file.

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 11, 2026 04:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The new targets are registered consistently and their setup, validation, and cleanup paths match the existing TBOR contracts.

0 open findings

🧠 Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants